Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit test summary jobs to sharktank and shortfin workflows. #695

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

ScottTodd
Copy link
Member

Progress on #357.

This gives us a single job in each unit test workflow to set as a "required check" that will block merging, replacing the current single Unit Tests (3.11, 2.3.0, ubuntu-24.04) check:
image

I'm also switching ci-libshortfin.yml to run on all changes, which is a requirement for making a check required. We could still conditionally skip jobs based on paths modified... just not using GitHub's built in path filtering.

@ScottTodd ScottTodd marked this pull request as ready for review December 13, 2024 22:01
@ScottTodd ScottTodd requested a review from marbre December 13, 2024 22:01
Comment on lines 157 to 168
- name: Getting failed jobs
run: |
echo '${{ toJson(needs) }}'
FAILED_JOBS="$(echo '${{ toJson(needs) }}' \
| jq --raw-output \
'map_values(select(.result!="success" and .result!="skipped")) | keys | join(",")' \
)"
echo "failed-jobs=${FAILED_JOBS}" >> $GITHUB_OUTPUT
if [[ "${FAILED_JOBS}" != "" ]]; then
echo "The following jobs failed: ${FAILED_JOBS}"
exit 1
fi
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed jobs list code is forked from IREE. We could simplify for cases where there is only one job or we aren't using the GITHUB_OUTPUT (e.g. to send an alert to an external service).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there are plans to send it out via GITHUB_OUTPUT thus simplifying sounds good to me and if really needed we know where to fork the complexity from 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll simplify. Will need to test a bit :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just echo "The job matrix failed" I guess...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh. Supporting a list of jobs is still useful for the future. I'll just remove the currently unused echo "failed-jobs=${FAILED_JOBS}" >> $GITHUB_OUTPUT.

@ScottTodd ScottTodd merged commit 0660b07 into nod-ai:main Dec 16, 2024
23 checks passed
@ScottTodd ScottTodd deleted the ci-job-anchors branch December 16, 2024 22:35
IanNod pushed a commit to IanNod/SHARK-Platform that referenced this pull request Dec 17, 2024
…ai#695)

Progress on nod-ai#357.

This gives us a single job in each unit test workflow to set as a
"required check" that will block merging, replacing the current single
`Unit Tests (3.11, 2.3.0, ubuntu-24.04)` check:

![image](https://github.com/user-attachments/assets/f8cc1101-606a-44ac-b467-f2d80fc45357)

I'm also switching ci-libshortfin.yml to run on all changes, which is a
requirement for making a check required. We could still conditionally
skip jobs based on paths modified... just not using GitHub's built in
path filtering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants